Note that in 6.0 we could decide to delete the Index*AlreadyExistsException classes. We'd still need to keep them in 5.x for backwards compatibility. Let me know what you think and I'll adjust the PR respectively.

I think that @s1monw's thinking should be taken into consideration before this PR is integrated.

I think this PR should remove all the exceptions and replace it by ResourceAlreadyExistsException. We can't add new exceptions in minors since it's basically a wire protocol break. What we can do is to rename an existing exception to ResourceAlreadyExistsException and deprecate all the other ones we have. In 6.0 we can remove them and for 5.x we rename IndexAlreadyExistsException to ResourceAlreadyExistsException and deprecate the usage of IndexShardAlreadyExistsException and IndexTemplateAlreadyExistsException. I really think we can replace most of them with a simple IllegalStateException or IllegalArgumentException? Ideally, we don't add a new one at all :)

There are a few points here that I would like to understand better:

• IndexShardAlreadyExistsException has a 500 HTTP status code. The other two have 400. As the one is server error and the other two user error, should we keep them separate? Or can we simply make ResourceAlreadyExistsException have a variable status code?
• If we rename IndexAlreadyExistsException to ResourceNotFoundException in 5.x aren't we already breaking BWC? Should we consider keeping all the IndexXxxAlreadyExistsException for 5.x as they are and only remove them in 6.0?
• Whatever decision we make for this one, should we also apply it for XxxNotFoundException? There is IndexNotFoundException, ShardNotFoundException, AliasesNotFoundException and ResourceNotFoundException.

Whatever decision we make for this one, should we also apply it for XxxNotFoundException? There is IndexNotFoundException, ShardNotFoundException, AliasesNotFoundException and ResourceNotFoundException.

lets not worry about this for now

IndexShardAlreadyExistsException has a 500 HTTP status code. The other two have 400. As the one is server error and the other two user error, should we keep them separate? Or can we simply make ResourceAlreadyExistsException have a variable status code?

I just looked into the code and replaced IndexShardAlreadyExistsException with IllegalStateException which maps to 500.. yet, I don't think this is important since it won't bubble up anyway. I also removed IndexTemplateAlreadyExistsException which is an IllegalArgumentException IMO. I can open up a PR soon for those two

If we rename IndexAlreadyExistsException to ResourceNotFoundException in 5.x aren't we already breaking BWC? Should we consider keeping all the IndexXxxAlreadyExistsException for 5.x as they are and only remove them in 6.0?

so on the wire protocol it will be just fine since we are not name-aware, we use an integer ID to deserialize. If you are thinking of REST layer we can just do something like this:

/*
 * Licensed to Elasticsearch under one or more contributor
 * license agreements. See the NOTICE file distributed with
 * this work for additional information regarding copyright
 * ownership. Elasticsearch licenses this file to you under
 * the Apache License, Version 2.0 (the "License"); you may
 * not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *    http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing,
 * software distributed under the License is distributed on an
 * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 * KIND, either express or implied.  See the License for the
 * specific language governing permissions and limitations
 * under the License.
 */

package org.elasticsearch.indices;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.index.Index;
import org.elasticsearch.rest.RestStatus;

import java.io.IOException;

public class ResourceAlreadyExistsException extends ElasticsearchException {

    public ResourceAlreadyExistsException(Index index, String message) {
        super(message);
        setIndex(index);
    }

    public ResourceAlreadyExistsException(StreamInput in) throws IOException{
        super(in);
    }

    @Override
    public RestStatus status() {
        return RestStatus.BAD_REQUEST;
    }

    @Override
    protected String getExceptionName() {
        if (getIndex() != null) {
            // this is only for BWC and can be removed in 6.0.0
            return "index_already_exists_exception";
        } else {
            return super.getExceptionName();
        }
    }
}

Sounds good, I will update the PR.

I summarise the decision as I understand it:

• 6.0: Rename IndexAlreadyExistsException to ResourceAlreadyExistsException
• 5.x: Rename IndexAlreadyExistsException to ResourceAlreadyExistsException but maintain the exception name to index_already_exists_exception when an index is set.

++ sounds good to me @jasontedor @nik9000 WDYT?

Makes sense to me.

++ sounds good to me @jasontedor @nik9000 WDYT?

Makes sense to me.

+1

PR is now updated with latest plan.

for the backport I think we need to add a BWC layer here other than that this change LGTM...

I opened a backport PR at #21601.

For those wondering how this turned out, yes it did break backwards compatibility for us as an ES API consumer.

https://review.opendev.org/#/c/724460

will rpm of version 6 with this change: a75320f soon be available in your yum repo